Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add hierarchySeparator to README #1445

Merged
merged 4 commits into from
Jul 15, 2017
Merged

Conversation

joscha
Copy link
Member

@joscha joscha commented Jul 10, 2017

Add hierarchySeparator example.

@joscha joscha added this to the v3.2.0 milestone Jul 10, 2017
@shilman
Copy link
Member

shilman commented Jul 10, 2017

@joscha @igor-dv @ndelangen vote for '/' as the one we push users towards. And I think Norbert doesn't even want it to be configurable?

@codecov
Copy link

codecov bot commented Jul 10, 2017

Codecov Report

Merging #1445 into release/3.2 will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##           release/3.2    #1445   +/-   ##
============================================
  Coverage        20.36%   20.36%           
============================================
  Files              236      236           
  Lines             5136     5136           
  Branches           634      721   +87     
============================================
  Hits              1046     1046           
+ Misses            3609     3546   -63     
- Partials           481      544   +63
Impacted Files Coverage Δ
addons/storyshots/src/storybook-channel-mock.js 0% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/error_display.js 0% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
addons/knobs/src/components/types/Number.js 8.06% <0%> (ø) ⬆️
lib/ui/src/modules/ui/libs/hierarchy.js 47.16% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/init_panels.js 25% <0%> (ø) ⬆️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
addons/info/src/components/Node.js 39.43% <0%> (ø) ⬆️
lib/ui/src/modules/api/configs/init_api.js 40.47% <0%> (ø) ⬆️
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0121a7c...fdcacc8. Read the comment docs.

@joscha
Copy link
Member Author

joscha commented Jul 10, 2017

I am fine either way - I have a preference for ., but can arrange myself with anything else as well :)

@igor-dv
Copy link
Member

igor-dv commented Jul 10, 2017

At the current project I am working on, we use '.' syntax, that actually was inspired by this. But there is nothing tragic to change it.
(Maybe worth it to change the docs as well)

@ndelangen why not to expose this as a setting ?

@ndelangen
Copy link
Member

ndelangen commented Jul 11, 2017

Having it configurable makes life more difficult for us in the long run.

Imagine webpack inline-loaders-syntax being configurable, what a mess that would quickly turn into.
We need to be careful to add special syntax into a string. It needs to make sense.

I don't think it makes a whole lot of sense to make this configurable other than, in 1 documentation-page we advocate for another character.

From what I hear everyone is actually OK with the / character, but some slightly prefer a .

I think we need a good why to increase the api-surface.

How about this: I'll do a poll on twitter

@joscha
Copy link
Member Author

joscha commented Jul 11, 2017

How about this: I'll do a poll on twitter

sounds good to me!

@ndelangen
Copy link
Member

https://twitter.com/storybookjs/status/884749480259710976

@joscha
Copy link
Member Author

joscha commented Jul 11, 2017

One question that just occured to me - dots have been advocated a while: https://storybook.js.org/basics/writing-stories/#prefix-with-dots that's why we initially chose them. Has that changed? Is there special logic anywhere to do matching against dots in the search?

EDIT: fuse.js seems to be fine with any separator...

@danielduan
Copy link
Member

danielduan commented Jul 13, 2017

Is there a middle ground we can go for? maybe / as default with an option for ., -, or _?

I can definitely see this getting messy with i18n, especially if things like the East Asian periods and Arabic reversed commas.

Copy link
Member

@ndelangen ndelangen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's just go for it.

Please change default to /

@joscha
Copy link
Member Author

joscha commented Jul 14, 2017

changed to /, however in docs only.

@igor-dv
Copy link
Member

igor-dv commented Jul 14, 2017

I'll add the default in code later today/tomorrow

@shilman
Copy link
Member

shilman commented Jul 15, 2017

Merging and leaving @igor-dv for the code followup 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants